refactor(data): require isEval on every helper that writes to the dataset - #129
Open
addyCooks wants to merge 2 commits into
Open
refactor(data): require isEval on every helper that writes to the dataset#129addyCooks wants to merge 2 commits into
addyCooks wants to merge 2 commits into
Conversation
…aset The `--eval` edit bug in Nano-Collective#113 was possible because `isEval = false` is defaulted on the mutating helpers in `src/lib/data.ts`. Two branches developed in parallel: Nano-Collective#80 threaded the flag through every call site that existed on its branch, Nano-Collective#96 added two new ones it never saw. Git raised no textual conflict and the type checker raised nothing either, so `updateTrainingExample(editIndex, updated)` compiled and quietly overwrote the training example at the same row index. The two-line call-site fix landed in 4543bbd. This closes the hole that let it happen: `isEval` is now a required parameter on every helper that writes into the dataset, so the next dropped flag is a compile error rather than a deleted example. Required: appendTrainingExample, appendToTrainingData, saveTrainingData, deleteExample, updateTrainingExample, dedupeExamples, fixContextMessages, importFromCSV, importFromJSONL, importFromJSON, importData. Read-only helpers keep the `isEval = false` default — countExamples, loadTrainingData, validateTrainingData and the exporters — where a missing flag shows the wrong set but destroys nothing. The compiler flagged one production call site, chat's `/keep`, which now passes `false` explicitly; every other caller already threaded the flag. The remaining updates are test call sites, all passing the value that was previously defaulted, so no behaviour changes.
…two lines The coverage gate failed on the previous commit: 71.65% -> 71.58%. The drop came entirely from `chat.tsx`, where wrapping the `/keep` call across four lines and adding a two-line comment put five new lines into a handler with no test coverage, in a file that sits at 19.8%. Uncovered lines added to a low-coverage file pull the repo-wide percentage down even though nothing about the change is untested in substance. Reduced that to a single extra line by naming the payload instead of letting the formatter wrap the argument list, which reads better than the cramped one-liner biome would otherwise reflow. The remaining margin was still under 0.03pp, so this also covers six genuinely untested branches in the importers whose signatures the previous commit changed. `Empty input or output`, `Unrecognized format` and the `missing user/assistant` skip paths had no tests at all, and neither did `importFromCSV` on an empty file or `importData` dispatching a `.json` extension. Each asserts that an unusable row is counted in `skipped` rather than dropped silently. data.ts coverage 96.05% -> 97.86%, repo-wide 71.61% -> 71.85% measured against the parent commit on the same machine.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Follow-up to #113. The two-line call-site fix landed in 4543bbd; this closes the
hole that let it happen.
isEval = falsewas defaulted on the mutating helpers insrc/lib/data.ts,which is why the parallel merge dropped it with no textual conflict and no type
error. #80 threaded the flag through every call site that existed on its branch;
#96 added two new ones it never saw.
updateTrainingExample(editIndex, updated)compiled fine and quietly overwrote the training example at the same row index.
isEvalis now a required parameter on every helper that writes into thedataset, so the next dropped flag is a compile error rather than a deleted
example:
appendTrainingExample,appendToTrainingData,saveTrainingData,deleteExample,updateTrainingExample,dedupeExamples,fixContextMessages,importFromCSV,importFromJSONL,importFromJSON,importData.Read-only helpers keep the
isEval = falsedefaultcountExamples,loadTrainingData,validateTrainingDataand the exporters where a missingflag shows the wrong set but destroys nothing. The asymmetry is documented at the
top of the write section.
I took the line at "writes into the dataset" rather than only the destructive
writes: a dropped flag on
importDataimports a validation file intotrain.jsonl, which is the same train/validation-separation failure the issuedescribes.
No behaviour changes
The compiler flagged exactly one production call site chat's
/keep, whichhas no
--evalcounterpart and now passesfalseexplicitly. Every other calleralready threaded the flag. I also hand-audited the reader call sites the type
checker can't catch (
data/add,data/export,data/import,data/list,data/validate,train) and they are all correct.The remaining 83 updates are test call sites, each passing the value that was
previously defaulted.
Type of Change
Testing
Automated Tests
pnpm test:allcompletes successfully)Manual Testing
nanotune initnanotune datacommands (add/import/list/validate)nanotune trainnanotune exportnanotune benchmarkChecklist
pnpm format)